-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Slider] Create unstyled version and migrate to emotion & styled-components #22435
Conversation
@@ -20,30 +21,56 @@ export default function ContinuousSlider() { | |||
setValue(newValue); | |||
}; | |||
|
|||
const theme = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes are added just for testing, will be reverted before merging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A reminder to remove the change from the demo. Maybe we could transform that into tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that the components are moved to the lab these are separate examples, but it makes sense to move these in the tests now :))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added tests, not sure if it makes sense to keep that per component, but at least we have it for now to make sure we don't regress during development
const Slider = React.forwardRef(function Slider(inputProps, inputRef) { | ||
const props = useThemeProps(inputProps, inputRef, 'MuiSlider'); | ||
const classes = useThemeClasses('MuiSlider'); | ||
const themeVariantsClasses = useThemeVariants({ ...props, classes }, 'MuiSlider'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slider is not really supporting variants, but it is added here for POC that it will work
Just a couple of thoughts:
|
Totally, will rename them. I am still thinking of potential names for the
I'd like ideally the unstyled or base version of component to know nothing about our styling mechanism, so people can opt out and style it from scratch, or even choose a different styling engine, maybe plain css, or something else. On the minimally styled version, I think maybe a better approach to be to export a theme that would provide that, rather than different version of the component. This way we may decide to add "Switch theme" capability for showing different themes on our docs page. These are just thoughts I have at this moment, I may change my mind, so will definitely keep this in mind |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A first quick review
The idea would be to provide a baseline style on which a theme would build, not a complete theme in its own right. (See Reach UI & Reakit). For example: SliderBase - unstyled; Slider - baseline styling; default theme - Material-UI. |
@mbrookes Would developers use it? For instance, if we look at the open source projects using Reakit and Reach UI on GitHub, e.g WordPress or welcome-ui what can we conclude? Do they use the baseline styles too? Without looking, my intuition would be that they don't and that it goes against the motivation for using the unstyled components in the first place. I would be happy to be proven wrong. We could also ask @gregberge as as happy user of Reakit: How do you best enjoy styling Reakit? Starting from a blank state of from their default styles? I think that we can solve this baseline style with a second theme, inspired by Tailwind, Bootstrap, and Chakra UI: no shadows, system font, outlined two pixels, etc. Something simple. |
Here's an unstyled slider: (I left the label in so that you can see where the slider is.) Where do you start with customising that?! But it isn't for me to speak for others – and asking one user what they prefer is the worst kind of anecdata! (No offence to @gregberge). Instead, I'm proposing an approach which provides unstyled, a baseline, and a fully baked theme as the starting point for customisation - a "win win win" situation.
Those are two complete (if minimal) design systems. (Two because Chakra is basically Tailwind, so why mention it?). Instead, I'm suggesting the possibility of components that work "out of the box", but are otherwise design-system agnostic (if unthemed). |
@mbrookes True. I imagine the problem would be identical to many components. If you take the data grid, good luck if you start from a blank state like the slider. We could ask developers on #6218. This is an interesting source to benchmark: JedWatson/react-select#2706. I wonder if a baseline theme, something that is easier to customize really exists for the slider or the data grid. Where would the simplification happen and make a non-negligible difference? Maybe it's about the inversion of control, maybe if we provide a simple demo with the styles exposed, using the unstyled slider, we would hit the target 🎯. |
That could be a good middle ground. 👍 |
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
I have turned the above exploration into #22435. I don't understand how the server-side works with third-party packages without the I have looked at emotion integration with Next.js. I don't think that the current setup is OK: I have tested, we can solve the issue with the "official" integration example. It uses a custom cache to step emotion to inject CSS inside the body and an extractor function to inline it in the head. @Andarist are the imports going to change in emotion v11? Are you aware of this line? |
We are aware about this validator reports but this doesn't have any downsides in practice. This strategy is allowing the so-called 0 config SSR because we can just render (and stream the result!) without any extra post processing steps. If you want style elements to be put in
Yes, but only a little bit and we provide a codemod for this. Changes are - different pkg name (
I assume that this is pretty much equivalent of: import createCache from '@emotion/cache'
const cache = createCache({ key: 'mui' })
cache.compat = true The only utilize the builtin cache from |
I have found whatwg/html#1605 on this matter. It might not have downside effects in practice, however, I have a concern with the noise it creates in the output, making it harder for us to identify other fails in https://validator.w3.org/. |
@oliviertassinari improved the next.js + emotion integration was fixed with those changes 👍 |
This PR has a lot of comments and I can't find the one that I would like to comment so... just going to do it here. I've consulted Mitchell on why
|
It can also lead to bundling |
Requested changes implemented
This pull request is meant to figure out most of the tradeoffs required to deliver the migration to emotion. The
Slider
component is used as a test case. Things done in the PR:SliderUnstyled
component, which basically contains all business logic of the component, plus generation of utility classes that developers can used for styling it.Slider
component, which is basically adding the material design styles on top of theSliderUnstyled
component, by creating a styled component for the root of the Slider.muiStyled
factory that is wrapper around thestyled
utilities supported that adds on top of the original styles the overrides and variants defined in the theme.@emotion/styled
- used by default in the corestyled-components
- intended for the developers that usestyled-components
. Developers should configure alias for the@material-ui/styled-engine
to point to@material-ui/styled-engine-cs
. (see example usage in the babel.config.js for the docs)Additional comments:
While reviewing, please consider reviewing the styled-component implementation as well, by commenting line 24 in the
docs/babel.config.js
and uncommenting line 25.Default emotion config:
styled-components config:
Next steps
StyledSlider
, should we use theSliderUnstyled
API? Is one example for theSliderUnstyled
enough (probably yes).peerDependencies
of@material-ui/styled-engine
should be added in the@material-ui/core
[Slider] Create unstyled version and migrate to emotion & styled-components #22435 (comment)